Fix prefix logging#20429
Merged
jasontedor merged 4 commits intoelastic:masterfrom Sep 13, 2016
jasontedor:logging-prefix-by-wrapping
Merged
Fix prefix logging#20429jasontedor merged 4 commits intoelastic:masterfrom jasontedor:logging-prefix-by-wrapping
jasontedor merged 4 commits intoelastic:masterfrom
jasontedor:logging-prefix-by-wrapping
Conversation
jasontedor
added
PITA
:Core/Infra/Logging
Today we add a prefix when logging within Elasticsearch. This prefix contains the node name, and index and shard-level components if appropriate. Due to some implementation details with Log4j 2 , this does not work for integration tests; instead what we see is the node name for the last node to startup. The implementation detail here is that Log4j 2 there is only one logger for a name, message factory pair, and the key derived from the message factory is the class name of the message factory. So, when the last node starts up and starts setting prefixes on its message factories, it will impact the loggers for the other nodes. Additionally, the prefixes are lost when logging an exception. This is due to another implementation detail in Log4j 2. Namely, since we log exceptions using a parameterized message, Log4j 2 decides that that means that we do not want to use the message factory that we have provided (the prefix message factory) and so logs the exception without the prefix. This commit fixes both of these issues.
| super(logger, name, null); | ||
| this.prefix = prefix; | ||
|
|
||
| final String actualPrefix = (prefix == null ? "[] " : prefix).intern(); |
Contributor
There was a problem hiding this comment.
this is a deviation from the 2.x version where a null prefix will be completely dropped. Are you sure this is a good change?
Member
Author
There was a problem hiding this comment.
Log4j will not allow a null marker, but I can replace this with the empty string if you prefer.
Contributor
There was a problem hiding this comment.
I think it will be cleaner? not a biggy though
Contributor
|
I'm not an expert here, but the approach looks like the best we had so far. Left some minor comments. |
When a logging prefix is null (e.g., when we do not yet know the node name), this commit causes such loggers to use an empty string marker instead of an empty prefix (i.e., "[] ") marker.
This commit fixes some failing logging tests. These tests are failing due to the changes of the logging prefix. Now that the logging prefix is used as a marker, we have to adjust the appender and mock appender conditions to account for the fact that the prefix is no longer part of the message.
This commit clarifies the intent of PrefixLogger by adding some comments, and addresses an issue in the de-referencing of a weak reference.
Contributor
|
LGTM. Thanks @jasontedor . Another fun adventure. |
Member
Author
|
Thank you @bleskes and @mikemccand for careful reviews. |
jasontedor
added a commit
that referenced
this pull request
Sep 13, 2016
Today we add a prefix when logging within Elasticsearch. This prefix contains the node name, and index and shard-level components if appropriate. Due to some implementation details with Log4j 2 , this does not work for integration tests; instead what we see is the node name for the last node to startup. The implementation detail here is that Log4j 2 there is only one logger for a name, message factory pair, and the key derived from the message factory is the class name of the message factory. So, when the last node starts up and starts setting prefixes on its message factories, it will impact the loggers for the other nodes. Additionally, the prefixes are lost when logging an exception. This is due to another implementation detail in Log4j 2. Namely, since we log exceptions using a parameterized message, Log4j 2 decides that that means that we do not want to use the message factory that we have provided (the prefix message factory) and so logs the exception without the prefix. This commit fixes both of these issues. Relates #20429
jasontedor
added a commit
that referenced
this pull request
Sep 13, 2016
Today we add a prefix when logging within Elasticsearch. This prefix contains the node name, and index and shard-level components if appropriate. Due to some implementation details with Log4j 2 , this does not work for integration tests; instead what we see is the node name for the last node to startup. The implementation detail here is that Log4j 2 there is only one logger for a name, message factory pair, and the key derived from the message factory is the class name of the message factory. So, when the last node starts up and starts setting prefixes on its message factories, it will impact the loggers for the other nodes. Additionally, the prefixes are lost when logging an exception. This is due to another implementation detail in Log4j 2. Namely, since we log exceptions using a parameterized message, Log4j 2 decides that that means that we do not want to use the message factory that we have provided (the prefix message factory) and so logs the exception without the prefix. This commit fixes both of these issues. Relates #20429
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Today we add a prefix when logging within Elasticsearch. This prefix
contains the node name, and index and shard-level components if
appropriate.
Due to some implementation details with Log4j 2 , this does not work for
integration tests; instead what we see is the node name for the last
node to startup. The implementation detail here is that Log4j 2 there is
only one logger for a name, message factory pair, and the key derived
from the message factory is the class name of the message factory. So,
when the last node starts up and starts setting prefixes on its message
factories, it will impact the loggers for the other nodes.
Additionally, the prefixes are lost when logging an exception. This is
due to another implementation detail in Log4j 2. Namely, since we log
exceptions using a parameterized message, Log4j 2 decides that that
means that we do not want to use the message factory that we have
provided (the prefix message factory) and so logs the exception without
the prefix.
This commit fixes both of these issues.